Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

chore: Update defaults to use legacy p2p #171

Closed
wants to merge 3 commits into from

Conversation

evan-forbes
Copy link
Member

Description

This PR changes the default configs to use the legacy p2p stack, along with adding default seed nodes.

I've tested this manually, but would appreciate if someone else does as well. After replacing the cosmos sdk with this branches latest commit in celestia-app, all we have to do to start a full node is

celestia-appd init --chain-id mamaki moniker
replace the genesis
celestia-appd start

Closes: #XXXX

@evan-forbes evan-forbes added the C: app Changes related to the celestia-app branches label May 27, 2022
@evan-forbes evan-forbes self-assigned this May 27, 2022
@evan-forbes evan-forbes changed the title Update defaults to use legacy p2p chore: Update defaults to use legacy p2p May 27, 2022
@codecov-commenter
Copy link

Codecov Report

Merging #171 (a63c024) into release/v0.46.x-celestia (cd6d960) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@                    Coverage Diff                    @@
##           release/v0.46.x-celestia     #171   +/-   ##
=========================================================
  Coverage                     65.13%   65.13%           
=========================================================
  Files                           682      682           
  Lines                         71680    71687    +7     
=========================================================
+ Hits                          46688    46695    +7     
  Misses                        22303    22303           
  Partials                       2689     2689           
Impacted Files Coverage Δ
x/genutil/client/cli/init.go 71.20% <100.00%> (+1.70%) ⬆️

Comment on lines +92 to +94
config.P2P.MaxNumInboundPeers = 250
config.P2P.MaxNumOutboundPeers = 70
config.P2P.MaxConnections = 320
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do these still reflect the latest thinking? I thought most validators decreased these values again. TBH, I am not sure we should hard-code these settings without very clear evidence that this is actually stabilizing the network.

Copy link
Member Author

@evan-forbes evan-forbes May 28, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree. Also as suggested earlier and in #151, we should move this to the app and core

Copy link
Member

@adlerjohn adlerjohn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the status of this proposal?

@evan-forbes
Copy link
Member Author

I'm not sure, as per #151, we should probably move these default settings the app or to core. However, if we move them to core, then that causes other issues as many tests in tendermint rely on the current default values, and if we move them to the app then that will require decently sized refactor/copy paste of code.

@evan-forbes
Copy link
Member Author

regardless, this needs to be updated to reflect the settings that we want, but I'm not sure that we have decided on what those are. converting to a draft for now

@evan-forbes evan-forbes marked this pull request as draft May 29, 2022 21:35
@evan-forbes
Copy link
Member Author

closing as this is no longer an option and hasn't been for a while

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C: app Changes related to the celestia-app branches
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants